Implement Oplog epoch fencing: oplog_metadata, epoch-checked appends, ShardLost relinquish - #3845
Aditya1404Sal wants to merge 10 commits into
Conversation
✅ Deploy Preview for golemcloud canceled.
|
da46c95 to
04f87cc
Compare
… ShardLost relinquish
f4830c0 to
d29fdfd
Compare
…h without deadlocks
| // The request reached an executor that does not own the agent's shard: a stale route, or an | ||
| // executor whose lease has lapsed. Not a refusal of the invocation - the caller retries it on | ||
| // the shard's owner after refreshing its routing table. | ||
| INVOCATION_REJECTION_REASON_SHARDING_NOT_READY = 15; |
There was a problem hiding this comment.
We already had an InvalidShardId case of WorkerExecutorError that was explicitly handled in the worker service layer by retrying with an updated routing table etc. I don't see how this fits into that?
| }, | ||
| /// The agent has been invoked | ||
| #[desert(evolution(FieldAdded("wallet_pin", None::<InvocationWalletPin>)))] | ||
| #[desert(evolution( |
There was a problem hiding this comment.
Don't add evolution steps until we start keeping backward compatibility. They are not free (increasing the serialization size and decreating deserialization performance)
| trace_states: Vec<String>, | ||
| invocation_context: Vec<SpanData>, | ||
| wallet_pin: Option<InvocationWalletPin>, | ||
| /// The shard epoch this executor held for the agent's shard when the invocation |
There was a problem hiding this comment.
The 'converting back yield None' part sounds problematic
Also we don't keep backward compatibility so there are no "entries written befor ethe fence existed"
| /// moving, or a write of its was fenced by the shard's new owner. Nothing is wrong with the | ||
| /// request, and a client that reconnects reaches the new owner - which is why this is not | ||
| /// `InternalError`. | ||
| ShardingNotReady, |
There was a problem hiding this comment.
Also see my previous comment about this error - but in case we need this new error, I still don't agree with its name :) 'shard is owned by another executor' does not mean 'sharding is not ready' - or maybe I misunderstand something?
| ); | ||
| } | ||
|
|
||
| /// Freezes this worker executor's process in place (SIGSTOP) without killing it: it keeps its |
There was a problem hiding this comment.
Is this helping with testing real-world scenarios? We already had sharding tests killing executor processes. Isn't that a better representation of what happens in production? (Loosing an executor node)
Is this for simulating other kind of failure scenarios?
| test_r::enable!(); | ||
|
|
||
| #[cfg(unix)] | ||
| mod unix { |
There was a problem hiding this comment.
Why do we need this and what (not) happens on windows?
| /// | ||
| /// The default does nothing and accepts everything: a backend that cannot fence has no record | ||
| /// to keep. | ||
| async fn upsert_oplog_metadata( |
There was a problem hiding this comment.
The indexed storage abstraction does not know about oplogs and we should not introduce that concept on this level. This makes it simpler to implement alternative storage backends.
That's why namespace is an abstract concept on this level and there is no logic in the indexed (and other) storage providers that depend on the actual use case.
| ); | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm a bit nervous about the amount of code changed in the oplog layers in general - I understand the introduction of reporting back fenced error instead of crashing the executor. But for all the other changes it is hard to me to see whether it's all necessary and keeping all the edge cases working as intended. Hopefully we have tests covering them - but I'd take another round to try to minimize the executor-level changes
| } | ||
| } | ||
|
|
||
| /// Appends one already-serialized compressed chunk, retrying transient failures like |
There was a problem hiding this comment.
Why do we need these changes?
| /// No: this executor owns the single shard for its whole life and nothing can take it away, | ||
| /// so there is no second writer to fence out. | ||
| fn requires_oplog_fencing(&self) -> bool { | ||
| false |
There was a problem hiding this comment.
Do we need this distinction? Can we make things simpler by not branching on tihs?
| use crate::worker::{ | ||
| CreateWorkerInstanceError, FinalWorkerState, PendingLiveInvocationDisposition, | ||
| PendingWorkerInterrupt, QueuedWorkerInvocation, RetryDecision, RunningAgent, | ||
| PendingWorkerInterrupt, QueuedWorkerInvocation, RelinquishReason, RetryDecision, RunningAgent, |
There was a problem hiding this comment.
I maybe wrong here (and the comment is about the whole invocation loop / Worker lifecycle part) but I'd expect this to be simpler with less changes. Why is the fencing error different than a regular non-retriable trap?
Sure there are some differences (as in not writing an Error oplog for example) but the worker lifecycle already supports this kind of stopping of the agent.
| cache_retirement_in_progress: AtomicBool, | ||
| /// Set once this executor has given the agent up. One-shot: the first reason wins, and the | ||
| /// agent is never revived here. | ||
| relinquishment: std::sync::OnceLock<RelinquishReason>, |
There was a problem hiding this comment.
Let's not use this word :) (anywhere) - it's not a common word, I had to look it up in a dictionary, this make the code hard to understand for non-english speakers.
| ArchiveWait, CommitLevel, EphemeralOplog, MultiLayerOplog, Oplog, OplogLifecycleGuard, | ||
| OplogOps, downcast_oplog, | ||
| ArchiveWait, CommitLevel, EphemeralOplog, MultiLayerOplog, Oplog, OplogError, OplogFence, | ||
| OplogLifecycleGuard, OplogOps, downcast_oplog, |
There was a problem hiding this comment.
The same general comment that I wrote to the invocation loop also applies to the changes in this file - it seems too much, touching everything - I cannot point to a particular set of lines that are unnecessary but I just feel this should not be like this
| warn!(%error, "Committing the oplog while stopping failed"); | ||
| false | ||
| } | ||
| }; |
There was a problem hiding this comment.
P2 — Classify the final commit’s fence before publishing pending-invocation failures.
The running-worker stop branch calls
fail_pending_invocationsbefore this final oplog commit. If this commit is the first operation to discover lost ownership, the stop has already cached and published an ordinary terminal failure rather than a routing error.For example: executor A begins deletion with an accepted invocation and a buffered oplog entry; executor B claims the oplog at a higher epoch before A’s invocation loop processes the interrupt. A publishes “Worker is being deleted” to its invocation waiters, then its final commit is fenced and marks the worker relinquished. The later relinquishment notification cannot repair this:
fail_pending_invocationsskips already-valid cached results before reaching its relinquished branch. The new owner can still execute the invocation, while its caller has received a terminal failure.Please perform the final commit and classify/mark any fence before failing pending invocations, while still completing the stop after refusal. A regression test should keep the guest from processing its interrupt, buffer an entry below the commit threshold, advance the stored epoch through a second writer, and drive an external stop carrying a non-routing error. Assert that callers receive a routing error and no ordinary invocation failure is cached. The buffered entry is important because an empty commit does not query storage.
Found by Amp’s oracle review; statically traced, not locally reproduced.
| .await | ||
| { | ||
| Ok(assignment) => { | ||
| self.carried_claim.write().unwrap().clear(); |
There was a problem hiding this comment.
P2 — Automatic epoch recovery also needs to recover delivery ordering.
A surviving executor retains its last-applied delivery revision when
clear_assignment()removes its shards. After a shard-manager store wipe, this re-registration can correctly repair shard epochs but return a revision below the executor’s retained revision.ShardService::registerpasses the grant throughShardAssignment::apply, which rejects it as stale; the outcome is ignored here and the carried claim is cleared.For example, an executor that applied revision 10,000 rejects a repaired registration at revision 3 and remains on an empty assignment. Subsequent deliveries are also rejected until the replacement manager’s revision catches up or the executor restarts. Restoring a backup that still recognizes the executor has the analogous problem on the renewal path. Renewals do advance the manager revision, so this is not necessarily permanent, but recovery is not bounded by the one renewal interval promised in the new deployment documentation.
The revision gate predates this PR; this is an incomplete integration of the newly added automatic history-repair feature, rather than a new revision-ordering regression. Please add tests with a high previously applied revision and a low repaired grant for both wiped-store re-registration and restored-store renewal. The current registration test helper always returns revision 1 and misses this case.
Repair needs to preserve delivery ordering across the reset—for example, by carrying last-applied revision evidence and minting repair deliveries above it, or by introducing a manager-incarnation ordering domain. Simply resetting the local revision must not allow delayed old-manager deliveries to take effect.
Found by Amp’s review and confirmed by the oracle; statically traced, not locally reproduced.
vigoo
left a comment
There was a problem hiding this comment.
Findings from an Amp-assisted review of the head commit (0b01ed2). Verified by reading the diff and running:
cargo check --testsfor golem-worker-executor, golem-shard-manager and golem-worker-service (clean);cargo test -p golem-shard-manager --lib(120 passed);cargo test -p golem-worker-executor --test integration -- indexed_storage::(194 passed across all five backends, Postgres via Docker). The e2e sharding tests were not run.Two items that have no diff line to anchor to:
golem-worker-service/src/service/worker/routing_logic.rs~L544: the retry-exhaustion branch computesdelayand logsdelay_msbut never sleeps. Pre-existing onmain, not introduced here — mentioning it because the PR body calls it out and it is trivially fixable while you are in the file.- Comment style overall: many comments narrate review history and previous shapes of the code ("the old
set_interrupting(..)shape risked", "F29", commit hashes). AGENTS.md asks for comments that describe the code as it is; please trim them in the next round.Checked and found correct, for the record: Postgres
SELECT … FOR UPDATECAS and the monotonicupsert_oplog_metadata; whole-batch rollback on SQLite/multi-SQLite; fence propagation through the multilayer/compressed/plugin layers; fork target opening unfenced; fencedAgentInvocationFinishednot being published; idempotency-key dedupe across a retry to the new owner;OplogError::Storagestill panics rather than being swallowed.
| indexed_storage: &(dyn IndexedStorage + Send + Sync), | ||
| indexed_storage_config: &IndexedStorageConfig, | ||
| ) -> anyhow::Result<()> { | ||
| if requires_oplog_fencing && !indexed_storage.supports_epoch_fencing() { |
There was a problem hiding this comment.
P1 — The shipped default config cannot boot a distributed executor after this lands.
The standalone executor always uses
GrpcShardManagerService(lib.rs:214), whoserequires_oplog_fencing()istrue, andgolem-worker-executor/config/worker-executor.toml(plus the sample env) still defaultsindexed_storage.typetoKVStoreRedis, which cannot fence. So an unmodified default deployment fails here at startup. The PR body defers the default change to "ticket 6", but merging leavesmainwith a non-booting default; CI won't notice because the test framework and docker-examples already use Postgres/SQLite.Please either change the shipped default in this PR or downgrade this guard to a warning until the default moves.
| @@ -2446,6 +2449,9 @@ fn safe_rejection_message(code: PublicErrorCode) -> String { | |||
| PublicErrorCode::ProducerError => "stream producer failed", | |||
| PublicErrorCode::InvocationFailed => "invocation failed", | |||
| PublicErrorCode::ProtocolError => "invocation protocol failed", | |||
| PublicErrorCode::ShardingNotReady => { | |||
There was a problem hiding this comment.
P1 —
sharding-not-readyis sent withretryable: false.The rejection frame builder (around line 1793 in this file) uses
retryable: matches!(code, PublicErrorCode::ResourceExhausted), so this new code shipsretryable: falsewhile the message here tells the client to retry. A client honouring the flag gives up on a transient condition. Please includeShardingNotReadyin the retryable set (and add it to the error-code table indocs/src/content/next/invoke/stream-session-public-protocol-v1.mdx, which does not list it).
| _ => match self.shard_epochs.get(&shard_id) { | ||
| Some(last) => last.next(), | ||
| None => ShardEpoch::initial(), | ||
| Some(last) => last.checked_next().filter(|epoch| epoch.0 != u64::MAX), |
There was a problem hiding this comment.
P2 — Ceiling handling is inconsistent between floor-raise and mint.
raise_epoch_floor_foraccepts and storesu64::MAX - 1(the test at ~1523-1535 asserts this), but this mint refuses to produceu64::MAX, so a shard sitting atMAX - 1can never change owner again. Either refuseMAX - 1in the floor raise too, or let the mint returnMAXand refuse only the following one. Practically unreachable for honest executors, but see the trust comment on the floor raise: a client can put a shard there deliberately.
| continue; | ||
| }; | ||
| self.shard_epochs.insert(*shard_id, epoch); | ||
| if let Some(entry) = self.shard_assignments.get_mut(shard_id) { |
There was a problem hiding this comment.
P2 — Client-supplied epochs are trusted verbatim (design question).
previous_shard_epochson register andfenced_shard_epochson renew are applied here without checking that the caller owns the shard. Any executor can therefore raise the floor on a shard owned by someone else (this branch rewritesentry.epochon the live assignment, forcing the real owner to relinquish it on the next renewal) or pin a shard atu64::MAX - 1. If executors are trusted peers this is acceptable, but please state that assumption in the shard-manager docs; a cheap hardening is to accept a floor raise only for shards the caller owns or that are unassigned.
| /// machine has an arm for each, so none is skipped. An agent still being resolved is not in | ||
| /// it, as a creation in progress never was: see `shard_epoch_to_assert` for why its oplog | ||
| /// cannot open unfenced on a shard that has already left the assignment. | ||
| pub(crate) async fn relinquish_matching( |
There was a problem hiding this comment.
P2 — The sweep misses agents that are still resolving, and this comment overstates the guard.
snapshot()only yieldsresolved_primary(). An agent that read the assignment before the revoke and then opened its oplog at the old epoch is never selected here, so it stays cached and unfenced until the new owner claims the metadata row.shard_epoch_to_assertdoes not cover it because the assignment it read still contained the shard. Durability is fine (the new owner's claim precedes its read of the last index, so late writes are fenced), but the agent lingers on this executor for the whole window. Consider re-running the sweep once resolution completes, or documenting this as a bounded liveness window instead of claiming it cannot happen.
| // write pool is capped at a single connection (golem-service-base | ||
| // db/sqlite.rs:46-50), so this transaction holds the only writer and the | ||
| // check cannot be interleaved. Raising that cap means switching this to | ||
| // `BEGIN IMMEDIATE`. |
There was a problem hiding this comment.
Nit — the single-writer claim holds per process only.
The one-connection write pool serialises writers inside this executor. Two executors sharing one SQLite file would rely on SQLite's lock upgrade from the deferred
begin()returningSQLITE_BUSY— safe, but that surfaces as a storage error, not a fence, and the comment does not say so. Either state that shared-file SQLite is unsupported for multi-executor deployments, or useBEGIN IMMEDIATEand say why.
| make_batch: DurableStreamBatchBuilder, | ||
| ) -> Result<Vec<(OplogIndex, OplogEntry)>, String> { | ||
| ) -> Result<Vec<(OplogIndex, OplogEntry)>, OplogError> { | ||
| let result = self.inner.add_durable_stream_batch(make_batch).await; |
There was a problem hiding this comment.
Nit — the rate limit is awaited before returning an already-fenced error.
If
inner.add_durable_stream_batchreturnedErr(OplogError::Fenced), there is nothing to rate-limit; return early onErrso a relinquishing worker does not sit in the limiter.
| // unfenced handle this service still holds at the same epoch is handed back without | ||
| // asking the storage, and then this is the only refusal before a write. | ||
| let fenced_at_create = match shard_epoch { | ||
| Some(epoch) => record_owning_epoch( |
There was a problem hiding this comment.
Nit —
create/create_freshupsert the owning epoch twice.This call records the epoch, and
open_withbelow callsrecord_owning_epochagain in thePrimaryOplogconstructor (~line 1143). Two round trips per create. More generally every open now costs one metadata write; consider a metric foroplog_metadataupserts/refusals so the cost is visible in production.
| ProducerError, | ||
| InvocationFailed, | ||
| /// The executor that answered does not own this agent's shard right now: the assignment is | ||
| /// moving, or a write of its was fenced by the shard's new owner. Nothing is wrong with the |
There was a problem hiding this comment.
Nit — typo: "a write of its was fenced" → "one of its writes was fenced".
| // reaped on a 20s tick, an unrenewed lease is gone within 80s of the pause. That bound | ||
| // keeps a thawed executor from waking up as the owner; it does not stretch the callers' | ||
| // retries, which count on the health check. | ||
| tokio::time::sleep(Duration::from_secs(90)).await; |
There was a problem hiding this comment.
Nit — this test does not deterministically prove a fenced write.
It relies on fixed sleeps (2 s here, 90 s below) and accepts either relinquish-by-assignment or a storage fence, asserting only that some epoch increased. The storage-level tests in
golem-worker-executor/tests/indexed_storage.rscover the fence itself, so this is a coverage note rather than a blocker, but a variant that holds the old owner frozen past the lease and then asserts a specificFencedrefusal on it would make the e2e meaningful.
|
I asked another agent to review my own comments above (not the agent-provided ones), pasting here the output, hoping it helps.
|
Resolves GOL-449
Implement Oplog epoch fencing: oplog_metadata, epoch-checked appends, ShardLost relinquish
Ticket 4 made shard ownership a lease. A lease bounds how long a lost shard keeps being served, but it
cannot stop an executor that has already lost the shard from finishing a write it had started. An
executor frozen by a pause, a VM migration or a partition wakes up still believing it owns its agents.
Until now the only thing standing between its writes and the new owner's was the
index_storageprimary key, which kills whichever executor happens to write second — often the rightful owner.
This PR records, per oplog, the shard epoch allowed to write it, and checks every batch of entries
against that record inside the transaction that inserts them. An executor whose epoch is behind is
refused at the storage. The one agent it was writing is given up (
ShardLost): nothing more is writtenfor it, and the worker service resumes it on the shard's owner. The executor keeps serving everything
else.
Builds on #3834. #3766 (revoke drain) lands after this and builds on
ActiveAgents::relinquish_matching.What this adds
The fence in storage
oplog_metadata (namespace, key, epoch)in the PostgreSQL andSQLite indexed storage (
002_oplog_metadata.sql). It is written before an oplog's first entry,removed before its entries, and only moves forward: the upsert is a compare-and-set
(
WHERE oplog_metadata.epoch <= EXCLUDED.epoch).with
SELECT ... FOR UPDATE; SQLite relies on its single-connection write pool (the code says whatraising that cap would require). A missing record refuses too, and a refused batch leaves nothing
behind. SQLite and multi-SQLite get their own transactional
append_many.IndexedStorage::append/append_manytakeshard_epoch: Option<ShardEpoch>.Noneasserts nothing: single-shard mode, the debugging service,a fork into a remote target, ephemeral oplogs and the archive layers. Backends that cannot fence say
so through
supports_epoch_fencing.A refused write gives up one agent
An oplog asserts the epoch its executor held when it opened it, claimed before the last index is
read, so an executor that already lost the shard is refused before it replays.
The refusal latches.
IndexedStorageError::Fenced→OplogError::Fenced→WorkerExecutorError::OplogFenced→TrapType::Interrupt(InterruptKind::ShardLost). After the firstrefusal every add, pair, start and commit on that handle is refused without asking the storage.
Entries buffered before the refusal stay readable, so a reader that took the index earlier never
finds a gap.
Refused writes are reported, never acknowledged.
Oplogand theWorkercommit APIs(
commit_oplog_and_update_state,add_and_commit_oplog) return the fence. Callers propagate it, sonothing is done for an entry the storage refused:
AgentInvocationFinishedis committed before a result is published to waiters;PendingAgentInvocationcommitted (unary and streaming);Startand the replayJumps gate the side effect they precede;Errorentry is refused traps instead of retrying the effect.Transient storage failures keep panicking exactly as before; only the fence changes behaviour.
Given up, never restarted in place. The agent is stopped without writing to its oplog or status
and removed from
ActiveAgents(scoped to its generation).OplogFencedcrosses the wire asShardingNotReady, which the worker service answers by refreshing its routing table and retrying onthe owner. Pending invocations of a given-up agent get that error and no cached result, because
the new owner runs them.
Revoked and narrowed shards give their agents up instead of restarting them with
InterruptKind::Restart, which would reopen the oplog at a stale epoch.The invocation loop never waits for a stop that waits for it. Once an agent is given up the loop
stops picking up work, never creates a new instance, and writes no lifecycle, failure, update or
streaming-session record for it. Loop-side cancel and manual-update enqueue refuse instead of waiting
when the worker is stopping.
A sweep never stalls lease renewal. Sweeps triggered by the renewal loop run on a single-flight
task, so a slow relinquish (for example of an agent still loading) cannot let the lease lapse.
AssignShardsand startup still await the sweep.Keeping epochs meaningful
raises the manager's floor. A re-registration after
LeaseNotFoundcarries the shards the executorheld (
RegisterRequest.previous_shard_epochs). Executors report the epochs their refused writes saw(
RenewShardLeaseRequest.fenced_shard_epochs), and the manager mints above them. Out-of-range epochsfrom the wire are ignored rather than overflowing.
AgentInvocationStartedrecords the epoch that wrote it (shard_epoch: Option<u64>, raw oplogonly; forks clear it).
INVOCATION_REJECTION_REASON_SHARDING_NOT_READYcarriesInvalidShardId,ShardingNotReadyandOplogFenced; the worker service retries it on the unary and streaming paths, and agent RPC maps itthe same way. Before,
InvalidShardIdfell through toInternal, a terminal error.(
ShardManagerService::requires_oplog_fencinghas no default on purpose).Supporting changes
cluster while it still believes it owns its shards, the case this PR is about.
and the rollout order; the persistence page lists PostgreSQL and SQLite as indexed storage.
worker-executor-walkthrough.htmland theunderstanding-durable-executionskill describeShardLost,the fence and fallible commits.
local-run/start.shruns the executor and the debugging service on SQLite indexed storage.Operational change and rollout
start, naming the setting to change (
GOLEM__INDEXED_STORAGE__TYPE:Postgres,Sqlite,KVStoreSqlite,MultiSqliteorKVStoreMultiSqlite). The shipped default is stillKVStoreRedis,so a distributed deployment on defaults fails at boot; changing the default belongs to ticket 6.
a routing miss with the new rejection reason, which only an upgraded worker service retries, and sends
epoch evidence in fields an older shard manager ignores.
002adds the table everyappend checks.
The ticket's plan, and how each part landed
The ticket had two halves: the executor side of the lease protocol, and oplog epoch fencing.
Lease protocol — landed in #3834 (ticket 4), not reopened here.
ExecutorIdper process, passed to every shard manager callLeaseNotFoundtakes a fresh idShardAssignmentcarriesshard_epochsand expiry;epoch_for_workerShardAssignment::epoch_of); this PR reads it when opening an oplogrenew_shard_lease/update_lease, background renewal loop at a third of the TTLStaleEpochorLeaseNotFound→ re-registerStaleEpoch: a mismatched claim is answered with the corrected setderegisteron graceful shutdownOplog epoch fencing — this PR.
oplog_metadata (namespace, key, epoch)tableupsert_oplog_metadataoverwrites a different epochdelete_oplog_metadataon worker deletionappend_manysignature unchanged; Postgres-only override takes the epochshard_epochon theIndexedStoragetraitArc<dyn IndexedStorage>; an inherent override is unreachableSELECT ... FOR UPDATEinside the insert transactionRepoError::OplogFencedFencedTxErrorcarrier insteadwith_tx_errneedsE: From<RepoError>, whichIndexedStorageErrordeliberately lacksIndexedStorageError::Fenced, not retriedShardEpochretry_storage_opreturns the fenceretry_oplog_append(which reconciles indeterminate writes) made fenceableRestartInterruptKind::ShardLost: give up, never restartcreate/openupsert before use, epoch from the worker's shardappend_manyfor SQLite and multi-SQLiteappendwrapped in a transactionappend_many; an unasserted single entry stays one autocommit insertAgentInvocationStarted.shard_epoch: u64(raw and public),0for old entriesOption<u64>, raw only0is a real epoch; the public oplog cannot see it anywayBeyond the ticket: the fallible
Oplogrefactor that makes a clean per-agent stop possible, therevoke/narrowing give-up, the startup guard, automatic epoch repair in the shard manager, the routing-miss
rejection reason, and the pause/resume test support with its end-to-end test.
Decisions made during the work
Design (before implementation)
panic-on-
Otherstays only for real storage failures). This required the fallibleOplogrefactor.restarting them. "Clean stop" plus "drain out of scope" would have built the stop and only fired it
after a wasted fenced write.
existing config; no new setting.
shard_epoch: Option<u64>.None). The default indexed storage staysKVStoreRedisfor now. No drain on local lease expiry — only definitive signals from the shard manager. Graceful
shutdown stays ticket 4's.
handles
Fencedlocally (relinquish or stop) and keeps the panic forOplogError::Storage.First review round
RegisterRequest.previous_shard_epochsand the renewal floor raise land here. The floor raisedeliberately reverses two ticket-4 test assertions: a claim above the record now re-mints above it.
drop_prefixbelong to Fix permanent shard loss when RevokeShards times out #3766.Final review round
relinquish_matchingand "relinquish awaits the loop'sexit" are unchanged for Fix permanent shard loss when RevokeShards times out #3766.
Review findings fixed
First review round — 41 verified findings: 39 fixed, 1 deferred (shipped default config → ticket 6),
1 not a defect (an old worker service rejects reason 15 as a protocol error, not a type-checker error).
pending_uploads.clear()on the fence path; a misplaced doc comment.AppendInvocationIfVersion's commit was reported as a successful append.ShardLost.rpc_error_from_rejectiondiscarded the executor's rejection text.shard_epochdoc named readers that cannot see it; forks copied the source's epoch; oldencodings without
shard_epochwere never decoded by a test.liveness too early.
signal_child's safety claim did not hold after the child was reaped; the pause API failedmisleadingly off unix; the 30 s lease override applied to every sharding test.
oplog-commit(replicas)reported durability after a fenced commit.002blocks rollback and a mid-rollout restart of an old executor (documented).EndAtomicRegionand fenced p3 request-body frames were classified as generic failures.ownership tests updated for give-up-on-revoke.
Final adversarial review — 38 candidates, 29 confirmed, from four root causes.
Refused commits were swallowed
AgentInvocationFinishedcommit.Startand three replayJumps ignored a refusal.holding the instance lock.
The primary oplog stayed half-alive after the fence latched
expects on invocation start/finish, HTTP responses,Worker::newand durable-sessionrecords now saw
Fencedand aborted the executor.Relinquish mechanics
Interrupted{ShardLost}.Ownership races
get_or_opendecided "I built this" from a shared flag; the dead-handle branch could remove a pendingreplacement. (The upstream merge replaced both with a per-agent lifecycle guard.)
u64::MAXoverflowed in the shard manager.Hygiene
debugging service read an empty Redis oplog; two tests could pass vacuously; an unchecked SQLite epoch
cast; an interpolated
warn!.Refuted: 6 (pre-existing or already decided). Not addressed: epoch reissue after a shard-manager store
wipe (follow-up, below) and a push lost when a handler is cancelled mid-persist (unconfirmed).
Review of the fixes (three reviewers, then a re-review)
deadlock).
FailedUpdateor a failed streaming session; replay toward anautomatic update could too.
handle could restart a given-up generation.
u64::MAXguard was off by one (u64::MAX - 1still overflowed on the next reassignment).Also fixed along the way
Found by the audits; each has a test.
hand a live owner's epoch to another executor; both passed the fence, because the record held only a
number. The record now names the writing process as well, an equal epoch from anyone else is refused,
and the refusal is reported so the manager mints past it. The writer is a per-process identity, not
the executor's lease identity, which is regenerated on
LeaseNotFoundand would fence a process offits own agents after every re-registration.
cast as a near-
u64::MAXepoch, was reported as evidence, and walked the manager's own mint to theceiling, where it panicked. The reads are checked, the mint is fallible - a shard whose epoch cannot
advance stays where it is instead of aborting the manager - and
LeaseEpochgained the checkedadvance its shard-side twin already had.
than
InternalError, so a session client reroutes like every other caller.RenewQuotaLease/ReleaseQuotaLeasecall with epochu64::MAXpanicked theshard manager. It is now an ordinary stale epoch.
instead of panicking on the key conflict.
Known gaps
Every open risk from the review was re-verified against this head. These are what is left.
Handed to #3766, which rewrites the revoke path this sits in:
suspend_workerstill flushes the status blob for an agent relinquished by a revoke, and statuswrites carry no epoch, so a stale one can overwrite the new owner's.
drop_prefixis not epoch-checked, so a transfer that outlives its ownership can still trim ordelete an oplog the new owner holds.
fence, stopping the attachment reconciler, draining the lifecycle, and
begin_deleteon the statusflusher and checkpointer. No durability impact - late writes are replayed or refused - but the
liveness gaps are real.
Also for #3766:
Worker::relinquishnow returns without its own stop when a deletion already ownsthe retirement, so the "relinquish awaits loop exit" contract no longer holds on that branch.
Accepted here:
epoch. The oplog - the only thing replay reads - stays correct; a stale status can be observed until
the agent's next status change on its new owner.
at-least-once exposure a crash has. A call that has not started is now refused before the effect
runs, including for writes marked idempotent, which previously checked nothing until the commit
after the effect.
deployment.
KVStoreRedis, which cannot fence, so a distributed deployment onunmodified defaults fails at startup until the setting is changed. Changing the default is ticket 6.
exhaustion it computes a backoff, never awaits it, resets the counter and loops. This PR routes a new
and more frequent failure into it, so a fence that does not resolve presents as a request that never
finishes. Worth fixing deliberately rather than inside a fence PR.
Resolved with evidence, not left open:
write runs on a spawned actor task and is queued before the caller's only await point, and the effects
guard poisons the producer rather than reporting success. Not a defect.
abort_transfer_in_dropisolder than this branch - and the orphan is swept by the next
drop_prefix.How it is tested
indexed_storage(5 backends, PostgreSQL in Docker)active_agents,hot_update,worker_initialization, ownership/fence/relinquish tests inapicargo make sharding-tests-debug)check-openapi(spec),check-configs,check-diff-model-fingerprint,check-witNonebypassing the check, and that a refused batch leaves nothing behind, on every backend(non-fencing backends must accept the same writes). Deliberately breaking each backend's check turns
the suite red.
its first write and stays refused; once latched, adds below the commit threshold are refused and
earlier indices stay readable; a writer holding a deleted oplog is refused; an indeterminate write on
a moved shard is reported as fenced; a fenced cached handle is never handed out again.
ShardLostand a transient storagefailure does not; every lost-shard exit path gives the agent up; a slow sweep does not stall renewal
and concurrent announcements coalesce; an invocation enqueued onto a fenced oplog is refused before
acceptance with
SHARDING_NOT_READY; a caller waiting on an invocation fenced inside a host call istold to reroute, and the executor writes nothing after it.
holds is refused and the refusal names the writer as the reason; the same process re-opening at the
same epoch is accepted, which is what happens on every cache eviction; and a newcomer minted above
the collision takes the oplog over, after which the old owner is the one refused. On the manager
side, a fence reported by a shard's own assignee at its own epoch is minted past, while the same
report from anyone else is the ordinary loser of a shard move and moves nothing.
finishes both ways round rather than deadlocking; a retry scheduled before a revoke does not resume
an agent this executor gave up; a start through a relinquished generation is refused and leaves the
generation that replaced it alone; a foreign stream mapping on a fenced oplog reports the fence,
commits nothing and releases the session lock.
(
an_executor_paused_until_its_shards_move_cannot_finish_the_invocations_it_started): eight agentseach start an in-flight invocation; three of four executors are frozen until their shards move; the
fourth takes the shards over and finishes the work; the frozen ones are thawed. Each invocation
returns exactly once, each agent gains exactly one
AgentInvocationFinishedfor the method, thestored owning epoch has moved, and every executor still serves afterwards. With the PostgreSQL append
check disabled the test fails: a thawed executor's stale append collides with the owner's and aborts it.
🤖 Generated with Claude Code